Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: prevent unauthorised remove-users access #3791

Merged

Conversation

matthewelwell
Copy link
Contributor

Thanks for submitting a PR! Please check the boxes below:

  • I have run pre-commit to check linting
  • I have added information to docs/ if required so people know about the feature!
  • I have filled in the "Changes" section below?
  • I have filled in the "How did you test this code" section below?
  • I have used a Conventional Commit title for this Pull Request

Changes

Prevents unauthorised users from removing users from a given organisation.

How did you test this code?

Added unit tests.

@matthewelwell matthewelwell requested review from a team and khvn26 and removed request for a team April 17, 2024 16:41
Copy link

vercel bot commented Apr 17, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 17, 2024 7:01pm
flagsmith-frontend-preview ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 17, 2024 7:01pm
flagsmith-frontend-staging ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 17, 2024 7:01pm

@github-actions github-actions bot added the api Issue related to the REST API label Apr 17, 2024
Copy link
Contributor

github-actions bot commented Apr 17, 2024

Uffizzi Preview deployment-50355 was deleted.

@codecov-commenter
Copy link

codecov-commenter commented Apr 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.92%. Comparing base (ec9e8ab) to head (cb68957).
Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3791   +/-   ##
=======================================
  Coverage   95.91%   95.92%           
=======================================
  Files        1102     1103    +1     
  Lines       34789    34822   +33     
=======================================
+ Hits        33369    33403   +34     
+ Misses       1420     1419    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


organisation_id = view.kwargs.get("pk")
if organisation_id and not organisation_id.isnumeric():
raise APIException("Invalid organisation ID")
Copy link
Member

@khvn26 khvn26 Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assumes the 500 status code. Why not use ValidationError instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops yep good catch, I wanted 400.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -1797,3 +1797,49 @@ def test_doesnt_retrieve_stale_api_usage_notifications(
# Then
assert response.status_code == status.HTTP_200_OK
assert len(response.data["results"]) == 0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a test for non-numeric org id.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@matthewelwell matthewelwell requested a review from khvn26 April 17, 2024 18:49
@matthewelwell matthewelwell added this pull request to the merge queue Apr 18, 2024
Merged via the queue into main with commit 05353a5 Apr 18, 2024
24 checks passed
@matthewelwell matthewelwell deleted the fix/prevent-non-org-admin-users-from-removing-users branch April 18, 2024 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Issue related to the REST API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants